-
Notifications
You must be signed in to change notification settings - Fork 4.1k
[Az.Aks] Enforce RSA as the GenerateSSHKeys default key type #28998
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status. |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request addresses compatibility issues with OpenSSH 9.4+ in the Azure Kubernetes Service (AKS) module by ensuring RSA SSH keys are generated instead of the new ed25519 default (which AKS doesn't support). The implementation adds version detection logic to conditionally specify the key type parameter when generating SSH keys.
Key Changes
- Added
GetOpenSSHVersion()method to detect the installed OpenSSH version through process execution and regex parsing - Modified
GenerateSshKeyValue()to conditionally add-t rsaflag for OpenSSH 9.4+ installations - Introduced platform-specific handling using
System.Runtime.InteropServicesfor Windows vs non-Windows path resolution
| string standOutput = process.StandardOutput.ReadToEnd() + process.StandardError.ReadToEnd(); | ||
|
|
||
| process.WaitForExit(); | ||
| // OpenSSH version for Windows follows the format: OpenSSH_for_Windows_X.XpX | ||
| // Examples | ||
| // "OpenSSH_for_Windows_8.6p1, LibreSSL 3.4.3" | ||
| // "OpenSSH_for_Windows_9.5p1, LibreSSL 3.8.2" | ||
| var regMatch = System.Text.RegularExpressions.Regex.Match(standOutput, @"OpenSSH_for_Windows_(\d+)\.(\d+)p(\d+)"); |
Copilot
AI
Dec 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable name "standOutput" appears to be a typo of "standardOutput". This inconsistent naming (compared to "standardError" later) reduces code clarity.
| string standOutput = process.StandardOutput.ReadToEnd() + process.StandardError.ReadToEnd(); | |
| process.WaitForExit(); | |
| // OpenSSH version for Windows follows the format: OpenSSH_for_Windows_X.XpX | |
| // Examples | |
| // "OpenSSH_for_Windows_8.6p1, LibreSSL 3.4.3" | |
| // "OpenSSH_for_Windows_9.5p1, LibreSSL 3.8.2" | |
| var regMatch = System.Text.RegularExpressions.Regex.Match(standOutput, @"OpenSSH_for_Windows_(\d+)\.(\d+)p(\d+)"); | |
| string standardOutput = process.StandardOutput.ReadToEnd() + process.StandardError.ReadToEnd(); | |
| process.WaitForExit(); | |
| // OpenSSH version for Windows follows the format: OpenSSH_for_Windows_X.XpX | |
| // Examples | |
| // "OpenSSH_for_Windows_8.6p1, LibreSSL 3.4.3" | |
| // "OpenSSH_for_Windows_9.5p1, LibreSSL 3.8.2" | |
| var regMatch = System.Text.RegularExpressions.Regex.Match(standardOutput, @"OpenSSH_for_Windows_(\d+)\.(\d+)p(\d+)"); |
| // OpenSSH version for Windows follows the format: OpenSSH_for_Windows_X.XpX | ||
| // Examples | ||
| // "OpenSSH_for_Windows_8.6p1, LibreSSL 3.4.3" | ||
| // "OpenSSH_for_Windows_9.5p1, LibreSSL 3.8.2" | ||
| var regMatch = System.Text.RegularExpressions.Regex.Match(standOutput, @"OpenSSH_for_Windows_(\d+)\.(\d+)p(\d+)"); |
Copilot
AI
Dec 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regex pattern only matches Windows-specific OpenSSH output format (OpenSSH_for_Windows_X.XpX), but will fail to detect the OpenSSH version on non-Windows platforms where the output format is different (e.g., "OpenSSH_9.4p1, OpenSSL 3.0.2"). This means the version detection will always return null on Linux/macOS, preventing RSA key type enforcement on those platforms when OpenSSH 9.4+ is installed. Consider using a platform-agnostic regex pattern that matches both formats, such as matching "OpenSSH_(\d+).(\d+)p(\d+)" which works for both "OpenSSH_9.4p1" and "OpenSSH_for_Windows_9.4p1".
| // OpenSSH version for Windows follows the format: OpenSSH_for_Windows_X.XpX | |
| // Examples | |
| // "OpenSSH_for_Windows_8.6p1, LibreSSL 3.4.3" | |
| // "OpenSSH_for_Windows_9.5p1, LibreSSL 3.8.2" | |
| var regMatch = System.Text.RegularExpressions.Regex.Match(standOutput, @"OpenSSH_for_Windows_(\d+)\.(\d+)p(\d+)"); | |
| // OpenSSH version formats to support: | |
| // Windows: "OpenSSH_for_Windows_X.XpX, LibreSSL ..." | |
| // Non-Windows:"OpenSSH_X.XpX, OpenSSL ..." | |
| var regMatch = System.Text.RegularExpressions.Regex.Match(standOutput, @"OpenSSH_(?:for_Windows_)?(\d+)\.(\d+)p(\d+)"); |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
| static Version GetOpenSSHVersion() | ||
| { | ||
| using (Process process = new Process()) | ||
| { | ||
| try | ||
| { | ||
| // Using runtime information to determine the path to ssh.exe based on OS type. | ||
| bool isWindows = RuntimeInformation.IsOSPlatform(OSPlatform.Windows); | ||
| process.StartInfo.FileName = isWindows ? "C:\\Windows\\System32\\OpenSSH\\ssh.exe" : "ssh"; | ||
| process.StartInfo.Arguments = "-V"; | ||
| process.StartInfo.UseShellExecute = false; | ||
| process.StartInfo.RedirectStandardInput = true; | ||
| process.StartInfo.RedirectStandardError = true; | ||
| process.StartInfo.RedirectStandardOutput = true; | ||
| process.StartInfo.CreateNoWindow = true; | ||
| process.Start(); | ||
|
|
||
| string standardOutput = process.StandardOutput.ReadToEnd() + process.StandardError.ReadToEnd(); | ||
|
|
||
| process.WaitForExit(); | ||
| // OpenSSH version for Windows follows the format: OpenSSH_for_Windows_X.XpX | ||
| // Examples | ||
| // "OpenSSH_for_Windows_8.6p1, LibreSSL 3.4.3" | ||
| // "OpenSSH_for_Windows_9.5p1, LibreSSL 3.8.2" | ||
| var regMatch = System.Text.RegularExpressions.Regex.Match(standardOutput, @"OpenSSH_for_Windows_(\d+)\.(\d+)p(\d+)"); | ||
|
|
||
| // We don't really care about the patch version, so only return major and minor version. | ||
| return regMatch.Success ? new Version(int.Parse(regMatch.Groups[1].Value), int.Parse(regMatch.Groups[2].Value)) : null; | ||
| } // We're not expecting ssh to be missing, but just in case, we catch any exception and return null. | ||
| catch | ||
| { | ||
| return null; | ||
| } | ||
| finally //Ensure process is properly disposed of and exited | ||
| { | ||
| if (!process.HasExited) | ||
| { | ||
| try | ||
| { | ||
| process.Kill(); | ||
| } | ||
| catch | ||
| { | ||
| // Ignore exceptions from Kill if process already exited | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
Copilot
AI
Dec 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new SSH key generation logic with version detection lacks test coverage. Given that the repository has comprehensive scenario tests for AKS functionality, this new feature should also have corresponding tests to validate:
- Correct version detection for different OpenSSH versions
- Proper handling when version detection fails (returns null)
- Correct argument construction for OpenSSH versions >= 9.4 vs. older versions
- Cross-platform behavior (Windows vs. Linux/macOS)
Consider adding unit tests or scenario tests that cover these SSH key generation scenarios.
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
@copilot open a new pull request to apply changes based on the comments in this thread |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
applying code review suggestions
|
/azp run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.
| static Version GetOpenSSHVersion() | ||
| { | ||
| using (Process process = new Process()) | ||
| { | ||
| try | ||
| { | ||
| // Using runtime information to determine the path to ssh.exe based on OS type. | ||
| bool isWindows = RuntimeInformation.IsOSPlatform(OSPlatform.Windows); | ||
| string defaultWindowsSshPath = @"C:\Windows\System32\OpenSSH\ssh.exe"; | ||
| process.StartInfo.FileName = isWindows && File.Exists(defaultWindowsSshPath) ? defaultWindowsSshPath : "ssh"; | ||
| process.StartInfo.Arguments = "-V"; | ||
| process.StartInfo.UseShellExecute = false; | ||
| process.StartInfo.RedirectStandardInput = true; | ||
| process.StartInfo.RedirectStandardError = true; | ||
| process.StartInfo.RedirectStandardOutput = true; | ||
| process.StartInfo.CreateNoWindow = true; | ||
| process.Start(); | ||
|
|
||
| string standardOutput = process.StandardOutput.ReadToEnd() + process.StandardError.ReadToEnd(); | ||
|
|
||
| process.WaitForExit(); | ||
| // OpenSSH version for Windows follows the format: OpenSSH_for_Windows_X.XpX | ||
| // Examples | ||
| // "OpenSSH_for_Windows_8.6p1, LibreSSL 3.4.3" | ||
| // "OpenSSH_for_Windows_9.5p1, LibreSSL 3.8.2" | ||
| var regMatch = System.Text.RegularExpressions.Regex.Match(standardOutput, @"OpenSSH_for_Windows_(\d+)\.(\d+)p(\d+)"); | ||
|
|
||
| // We don't really care about the patch version, so only return major and minor version. | ||
| return regMatch.Success ? new Version(int.Parse(regMatch.Groups[1].Value), int.Parse(regMatch.Groups[2].Value)) : null; | ||
| } // We're not expecting ssh to be missing, but just in case, we catch any exception and return null. | ||
| catch | ||
| { | ||
| return null; | ||
| } | ||
| finally //Ensure process is properly disposed of and exited | ||
| { | ||
| if (!process.HasExited) | ||
| { | ||
| try | ||
| { | ||
| process.Kill(); | ||
| } | ||
| catch | ||
| { | ||
| // Ignore exceptions from Kill if process already exited | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
Copilot
AI
Dec 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new OpenSSH version detection logic in GetOpenSSHVersion() lacks test coverage. Since the repository uses comprehensive automated testing for the Az.Aks module, consider adding unit tests to verify:
- Correct version parsing for different OpenSSH output formats (Windows and non-Windows)
- Handling of null returns when OpenSSH is not installed or version cannot be determined
- Correct behavior when version is below 9.4 vs. 9.4 and above
- The resulting ssh-keygen command arguments are correct for different scenarios
|
Azure Pipelines successfully started running 3 pipeline(s). |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 5 comments.
| // We're not expecting ssh to be missing, but just in case, we catch any exception and return null. | ||
| catch | ||
| { | ||
| return null; | ||
| } |
Copilot
AI
Dec 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The broad catch block on line 349 silently swallows all exceptions, which could hide important errors beyond just missing SSH binaries. Consider catching specific exceptions like FileNotFoundException or Win32Exception, and logging the exception details using WriteDebug or WriteWarning before returning null. This would help with troubleshooting when SSH version detection fails.
| // We're not expecting ssh to be missing, but just in case, we catch any exception and return null. | |
| catch | |
| { | |
| return null; | |
| } | |
| // We're not expecting ssh to be missing, but just in case, we catch specific exceptions and return null. | |
| catch (FileNotFoundException ex) | |
| { | |
| WriteDebug($"Failed to detect SSH version: SSH executable not found. Detail: {ex.Message}"); | |
| return null; | |
| } | |
| catch (Win32Exception ex) | |
| { | |
| WriteDebug($"Failed to detect SSH version due to a Win32 error when starting the SSH process. Detail: {ex.Message}"); | |
| return null; | |
| } | |
| catch (InvalidOperationException ex) | |
| { | |
| WriteDebug($"Failed to detect SSH version due to an invalid process configuration. Detail: {ex.Message}"); | |
| return null; | |
| } | |
| catch (Exception ex) | |
| { | |
| WriteDebug($"Failed to detect SSH version due to an unexpected error. Detail: {ex.Message}"); | |
| return null; | |
| } |
| string standardOutput = process.StandardOutput.ReadToEnd() + process.StandardError.ReadToEnd(); | ||
|
|
||
| process.WaitForExit(); |
Copilot
AI
Dec 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The process output reading on line 335 may cause a deadlock if the output buffers fill up. Reading both StandardOutput and StandardError synchronously after process.Start() can block if either buffer becomes full before WaitForExit is called. Consider using asynchronous reading with BeginOutputReadLine and BeginErrorReadLine, or read StandardError and StandardOutput separately to avoid potential deadlock situations.
| // From OpenSSH 9.4 onwards, the default key type is ed25519, which is not supported in AKS. | ||
| // To ensure backward compatibility, we need to check the OpenSSH version installed and adjust the parameters accordingly. | ||
| static Version GetOpenSSHVersion() | ||
| { | ||
| using (Process process = new Process()) | ||
| { | ||
| try | ||
| { | ||
| // Using runtime information to determine the path to ssh.exe based on OS type. | ||
| bool isWindows = RuntimeInformation.IsOSPlatform(OSPlatform.Windows); | ||
| string defaultWindowsSshPath = @"C:\Windows\System32\OpenSSH\ssh.exe"; | ||
| process.StartInfo.FileName = isWindows && File.Exists(defaultWindowsSshPath) ? defaultWindowsSshPath : "ssh"; | ||
| process.StartInfo.Arguments = "-V"; | ||
| process.StartInfo.UseShellExecute = false; | ||
| process.StartInfo.RedirectStandardInput = true; | ||
| process.StartInfo.RedirectStandardError = true; | ||
| process.StartInfo.RedirectStandardOutput = true; | ||
| process.StartInfo.CreateNoWindow = true; | ||
| process.Start(); | ||
|
|
||
| string standardOutput = process.StandardOutput.ReadToEnd() + process.StandardError.ReadToEnd(); | ||
|
|
||
| process.WaitForExit(); | ||
| // OpenSSH version output follows formats like: | ||
| // Windows: "OpenSSH_for_Windows_8.6p1, LibreSSL 3.4.3" | ||
| // Windows: "OpenSSH_for_Windows_9.5p1, LibreSSL 3.8.2" | ||
| // Linux/macOS: "OpenSSH_9.5p1, OpenSSL 3.0.13 30 Jan 2024" | ||
| // We match the common "OpenSSH_" prefix and make "for_Windows_" optional so this works across platforms. | ||
| var regMatch = System.Text.RegularExpressions.Regex.Match(standardOutput, @"OpenSSH_(?:for_Windows_)?(\d+)\.(\d+)"); | ||
|
|
||
| // We don't really care about the patch version, so only return major and minor version. | ||
| return regMatch.Success ? new Version(int.Parse(regMatch.Groups[1].Value), int.Parse(regMatch.Groups[2].Value)) : null; | ||
| } | ||
| // We're not expecting ssh to be missing, but just in case, we catch any exception and return null. | ||
| catch | ||
| { | ||
| return null; | ||
| } | ||
| finally //Ensure process is properly disposed of and exited | ||
| { | ||
| if (!process.HasExited) | ||
| { | ||
| try | ||
| { | ||
| process.Kill(); | ||
| } | ||
| catch | ||
| { | ||
| // Ignore exceptions from Kill if process already exited | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
Copilot
AI
Dec 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ChangeLog.md file is missing an entry for this SSH key generation change. According to the repository's contribution guidelines, you SHOULD update the ChangeLog.md file under the "Upcoming Release" section to document this change. Add a bullet point describing the enforcement of RSA as the default key type for OpenSSH 9.4 and above to ensure AKS compatibility.
| // Validate if OpenSSH version is 9.4 or above to add -t rsa argument | ||
| // Which has been defaulted to ed25519 from OpenSSH 9.4 onwards | ||
| // https://github.com/openssh/openssh-portable/blob/master/ssh-keygen.c#L70 | ||
| var openSshVersion = GetOpenSSHVersion(); | ||
| // If we cannot determine the OpenSSH version, we assume it's below 9.4 to maintain compatibility. | ||
| // If openSshVersion isn't null and is >= 9.4, we add the -t rsa argument, otherwise we skip it | ||
| var keyTypeArgument = openSshVersion != null && openSshVersion >= new Version(9, 4) ? "-t rsa " : ""; | ||
| process.StartInfo.FileName = "ssh-keygen"; | ||
| process.StartInfo.Arguments = String.Format("-f \"{0}\"", generateSshKeyPath); | ||
| // if keyTypeArgument is empty, we skip it to maintain compatibility with older OpenSSH versions | ||
| // Otherwise, we explicitly set the key type to rsa | ||
| process.StartInfo.Arguments = String.Format("{0}-f \"{1}\"", keyTypeArgument, generateSshKeyPath); |
Copilot
AI
Dec 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new OpenSSH version detection logic and conditional SSH key type generation are not covered by automated tests. Given that the Aks.Test project contains comprehensive test scenarios for other module functionality, this new behavior should have test coverage to verify correct version detection, version comparison logic, and proper argument construction for both OpenSSH versions below and above 9.4.
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
@copilot open a new pull request to apply changes based on the comments in this thread |
Description
This pull request enhances the SSH key generation logic in the
NewAzureRmAkscommand to ensure compatibility with changes in OpenSSH 9.4 and above, where the default key type switched toed25519(unsupported by AKS). The main update involves detecting the installed OpenSSH version and conditionally specifying the key type when generating SSH keys. Additionally, it introduces platform checks and error handling for robust version detection.SSH Key Generation Compatibility Updates:
GetOpenSSHVersion()to detect the installed OpenSSH version, using platform checks and safe process handling to extract the version fromssh.exeoutput.GenerateSshKeyValue()method to use the detected OpenSSH version: if version 9.4 or above is found, the-t rsaargument is added tossh-keygento ensure RSA keys are generated; otherwise, the argument is omitted for compatibility with older OpenSSH versions.Platform Support:
using System.Runtime.InteropServices;directive and platform checks to ensure the correct path tossh.exeis used on Windows and non-Windows systems.These changes help prevent issues with unsupported SSH key types in AKS clusters on systems with newer OpenSSH installations.
Mandatory Checklist
Please choose the target release of Azure PowerShell. (⚠️ Target release is a different concept from API readiness. Please click below links for details.)
Check this box to confirm: I have read the Submitting Changes section of
CONTRIBUTING.mdand reviewed the following information:ChangeLog.mdfile(s) appropriatelysrc/{{SERVICE}}/{{SERVICE}}/ChangeLog.md.## Upcoming Releaseheader in the past tense.ChangeLog.mdif no new release is required, such as fixing test case only.